refactor: #718 only drop TimestampSeries #1274

cmp0xff · 2025-07-13T06:40:48Z

Addresses CLEAN: Investigate whether TimestampSeries, TimedeltaSeries, etc. can be removed #718
Tests added: Please use assert_type() to assert the type of any return value

tests/test_timefuncs.py

pandas-stubs/core/series.pyi

tests/test_series.py

tests/test_frame.py

tests/test_scalars.py

Dr-Irv · 2025-07-23T16:53:15Z

@cmp0xff you have a number of PRs submitted while I was out on vacation for 2 weeks. Can you let me know which ones I should prioritize for review?

cmp0xff · 2025-07-23T18:15:12Z

Hi @Dr-Irv, I hope you had a nice vacation. My pull requests are categorised below. Each category is independent, but those in a higher position have a slightly higher priority in my opinion.

`Series`: arithmetic operations

The following two PRs are independent. They migrate test_series.py to a subfolder series, and add quite a few test_*.py files there.

add: feat(series): #1098 arithmetic addition #1275
truediv: feat(series): #1098 arithmetic truediv #1280

`DataFrame.to_dict`

fix(DataFrame): #799 to_dict #1283

`Index.append`

feat(index): append #1282

`Series`: address #718

refactor: #718 only drop TimestampSeries #1274 - this is a prerequisite for the next one.
refactor: #718 also drop TimedeltaSeries #1273

Dr-Irv

Thanks for doing this. It's a lot of good work.

Main thing - if I'm going to merge this PR, it needs to be in a state where we don't need the followup PR.

Basic rule - we don't put ignore in the tests unless we are testing that the stubs should not accept something that is invalid. You have places where you have added ignore in the tests and I won't merge that in (unless we know it is a bug in the type checker)

docs/philosophy.md

Dr-Irv · 2025-07-24T20:59:41Z

tests/test_frame.py

+    check(assert_type(s + summer, pd.Series), pd.Series)  # type: ignore[assert-type]
+    check(assert_type(s + df["y"], pd.Series), pd.Series)  # type: ignore[assert-type]
+    check(assert_type(summer + summer, pd.Series), pd.Series)  # type: ignore[assert-type]


don't want to have ignore in the tests. Fix the types to make this work.

These are not there anymore.

tests/test_frame.py

tests/test_scalars.py

tests/test_timefuncs.py

pandas-stubs/core/series.pyi

Dr-Irv · 2025-07-24T21:28:34Z

Hi @Dr-Irv, I hope you had a nice vacation. My pull requests are categorised below. Each category is independent, but those in a higher position have a slightly higher priority in my opinion.

I've reviewed them all, except #1273 as noted there.

Thanks for all the great work.

cmp0xff · 2025-07-24T22:45:42Z

I've reviewed them all, except #1273 as noted there.

Thanks for all the great work.

Thank you very much for your quick and thorough reviews. I will be able to work on them next week.

…les#r2229555145

…74/files#r2229550572

…les#r2229581983

cmp0xff · 2025-08-06T07:41:12Z

Hi @Dr-Irv @twoertwein, it is difficult to remove only TimestampSeries. If we do so, for example, Series[Timestamp] - Series[Timestamp] would give TimedeltaSeries. Having this, mypy and pyright have different opinions handling Series[Any] - Series[Any]. mypy gives Any, whereas pyright gives Series[Any].

Can we go back to the original plan, and remove both TimestampSeries and TimedeltaSeries in one PR?

Dr-Irv · 2025-08-06T15:34:09Z

Hi @Dr-Irv @twoertwein, it is difficult to remove only TimestampSeries. If we do so, for example, Series[Timestamp] - Series[Timestamp] would give TimedeltaSeries. Having this, mypy and pyright have different opinions handling Series[Any] - Series[Any]. mypy gives Any, whereas pyright gives Series[Any].

Can we go back to the original plan, and remove both TimestampSeries and TimedeltaSeries in one PR?

OK, you can try. But I think this may be something you can't fix due to how mypy is handling things, and that's why I had to create TimestampSeries and TimedeltaSeries in the first place.

…les#r2229580369

cmp0xff · 2025-08-07T15:45:16Z

Hi @Dr-Irv, I continued working on the current #1274 anyway. Please let me know if you think we can proceed to merge it, or if we need to do better. In the latter case, I believe I have to remove both TimestampSeries and TimedeltaSeries together.

Summary

Highlights

TimestampSeries removed
All except 1 existing tests pass
All review comments replied to

To-dos

1 test fails, inline commented with reasons, expected to be fixed when removing TimedeltaSeries
Series[Timedelta] is used at several places, expected to be fixed when removing TimedeltaSeries
Overloads of Series.__mul__ is suboptimal, can be fixed in a separate PR

Orphaned comments

#1274 (comment)

need to not be there now

code removed, see #1274 (comment)

#1274 (comment)

but you shouldn't have the ignores here. I'm really hesitant to accept this change in this PR without knowing that both PRs work together without this change.

explained in #1274 (comment)

#1274 (comment)

I don't understand this comment.

code removed, see #1274 (comment)

Dr-Irv

This is pretty close. There are 2 main issues reflected in the comments.

I think the changes you made for __sub__() and possibly __mul__() and related methods to handle int, float, etc., should be a separate PR, similar to what you did for _add__() and __div__(). If we can get that working, and then do this one, it will be easier to make sure all the tests are still working right.
I don't want to merge with any new ignore in the tests, because if someone pulls main, they will get something we know is broken. But I suggest a plan for handling that in the comments.

Thanks again for the great work on this.

Dr-Irv · 2025-08-07T18:26:33Z

tests/series/test_series.py

+        check(x.astype(cast_arg), pd.Series, target_type)
    else:
-        check(s.astype(cast_arg), TimestampSeries, target_type)
+        check(s.astype(cast_arg), pd.Series, target_type)


Shouldn't these both be pd.Series[pd.Timestamp] ?

Dr-Irv · 2025-08-07T18:30:54Z

tests/series/test_series.py

+    # Will be fixed after removing TimedeltaSeries, see Series.__sub__ in series.pyi
+    check(assert_type(ss, pd.Series), pd.Series)  # type: ignore[assert-type]


I'm don't want to merge until we deal with this, because everything in main should pass all the tests. But here is what I'd like to do. Once I'm OK with this PR, I won't merge it, and then the TimedeltaSeries PR can merge into this branch where the line above should get fixed.

I could put this branch into the repo once approved so it becomes the new PR target of the next PR, and then once that is approved and merged, we do a new PR from that branch to main

Dr-Irv · 2025-08-07T18:48:12Z

pandas-stubs/core/indexes/accessors.pyi

 # is invoked, but because of how Series.dt is hooked in and that we may not know the
 # type of the series, we don't know which kind of series was ...ed
 # in to the dt accessor

 _DTTimestampTimedeltaReturnType = TypeVar(
-    "_DTTimestampTimedeltaReturnType",
-    bound=Series | TimestampSeries | TimedeltaSeries | DatetimeIndex | TimedeltaIndex,
+    "_DTTimestampTimedeltaReturnType", bound=Series | DatetimeIndex | TimedeltaIndex


Not clear why TimedeltaSeries is removed at this point. Or shouldn't it be bound=Series | Series[Timestamp] | TimedeltaSeries | DatetimeIndex | TimedeltaIndex ?

I know that Series includes the other 2, but I'd like to keep this as close as possible to what was there before.

Dr-Irv · 2025-08-07T18:58:37Z

pandas-stubs/core/series.pyi

+    @overload
+    def __sub__(self: Series[S1C], other: Series[Never]) -> Series: ...
+    @overload
    def __sub__(
-        self: Series[Timestamp],
-        other: Timedelta | TimedeltaSeries | TimedeltaIndex | np.timedelta64,
-    ) -> TimestampSeries: ...
+        self: Series[int], other: _T_COMPLEX | Sequence[_T_COMPLEX] | Series[_T_COMPLEX]
+    ) -> Series[_T_COMPLEX]: ...
+    @overload
+    def __sub__(self: Series[int], other: np_ndarray_anyint) -> Series[int]: ...
+    @overload
+    def __sub__(self: Series[int], other: np_ndarray_float) -> Series[float]: ...
+    @overload
+    def __sub__(self: Series[int], other: np_ndarray_complex) -> Series[complex]: ...
    @overload
    def __sub__(
-        self: Series[Timedelta],
-        other: Timedelta | TimedeltaSeries | TimedeltaIndex | np.timedelta64,
-    ) -> TimedeltaSeries: ...
+        self: Series[float],
+        other: int | Sequence[int] | np_ndarray_anyint | np_ndarray_float | Series[int],
+    ) -> Series[float]: ...
+    @overload
+    def __sub__(
+        self: Series[float],
+        other: _T_COMPLEX | Sequence[_T_COMPLEX] | Series[_T_COMPLEX],
+    ) -> Series[_T_COMPLEX]: ...
+    @overload


I think these changes for __sub__() and __rsub__() and sub() should be in a separate PR like you did for add and div, with the tests like you did there.

Dr-Irv · 2025-08-07T19:02:40Z

pandas-stubs/core/series.pyi

+    @overload
+    def __rtruediv__(  # pyright: ignore[reportIncompatibleMethodOverride]
+        self: Series[Timedelta], other: _nonseries_timedelta | Series[Timedelta]
+    ) -> Series[float]: ...


I think you can delete the first __rtruediv__() declaration (or combine the 2) because it is returning the same result, and self is already a subclass of Series[Timedelta] . Not 100% sure of that

cmp0xff mentioned this pull request Jul 13, 2025

refactor: #718 also drop TimedeltaSeries #1273

Draft

2 tasks

cmp0xff marked this pull request as ready for review July 13, 2025 07:05

cmp0xff changed the title ~~fix: #718 only drop TimestampSeries~~ refactor: #718 only drop TimestampSeries Jul 13, 2025